WIP: Fix for incorrectly marked collected reference on X86 #3035

0dvictor · 2018-10-05T15:18:08Z

The result register of TR::aiadd/TR::aladd was incorrectly marked as a
collected reference, when the aiadd/aladd node is not internal pointer.
As a result, an invalid GC map entry may be introduced and causes GC assertions.
The result of TR::aiadd/TR::aladd can never be a collected reference, fixing it.

Signed-off-by: Victor Ding [email protected]

liqunl · 2018-10-05T15:35:07Z

The commit message is worrying me and seems wrong. The result of aiadd and aladd can be a collected reference if they're internal pointer. There is an API to compute the collectedness of a data. It's OMR::Node::computeIsCollectedReference. But this API might not work after lower tree since we do a lot of a2l.

0dvictor · 2018-10-05T16:10:09Z

@liqunl Could you give some examples when the result of TR::aiadd/TR::aladd should return a collected reference?
If the address node is a collected reference, the result of [address+offset] cannot be another collected reference; if the address node is not a collected reference, how can we possibly know the result is a collected reference?

liqunl · 2018-10-05T18:21:39Z

I was confused because there are similar concepts on symbol and on node, they are slightly different than the concept in codegen. On Node and Symbol, collected reference and internal pointer are both considered to be collected, but there is addition flag for internal pointer which is used by codegen to create gc map for it (when it also has a pinning array) but not to treat them as the real collected reference.

I agree that aiadd/aladd can't be collected refenrence regardless of whether they're internal pointers. I have no issue with the commit message now.

andrewcraik · 2018-10-05T18:37:33Z

@vijaysun-omr could you review? I think this is popping up a fair number of failures in recent builds.

vijaysun-omr · 2018-10-05T18:46:07Z

compiler/x/codegen/BinaryCommutativeAnalyser.cpp

-         else if (comp->generateArraylets() && root->getOpCodeValue() == TR::aiadd)
-            // arraylets: aiadd is technically internal pointer into spine object, but isn't marked as internal pointer
-            tempReg = _cg->allocateRegister();
-         else


I presume this allocateCollectedReferenceRegister section of code is the one that was causing problems ?
Could you please elaborate on the actual problem that you are fixing ?

The result register (tempReg here) was incorrectly marked as a collected reference, when the aiadd/aladd node is not internal pointer. As a result, an invalid GC map entry may be introduced and causes GC assertions.
I cannot think of any valid reasons that this result could be collected; therefore, all branches are now to allocate a "non-collected" register instead of "collected" ones.

It seems we would have used an uncollected register if the isInternalPointer flag would have been "true" on this aiadd/aladd. I expect this node flag would be set on all aiadd/aladd in Java but I guess you saw some case where that flag was not set (?) If so, how did such a node originate ?

It is from Value Propagation where a TR::arraycopy gets generated. The source and destination children are TR::aladd.

If these aladds created by arraycopy generation in value propagation don't have the internal pointer flag set, then we should consider setting the flag and see if that fixes the failure.

But either way aladd shouldn't return a collected reference. It doesn't make sense to create a collected reference from [address+offset].

Agreed, but it's preferable to fix a bug in common code if we can so that each platform does'nt have to make changes.

Good suggestion. @liqunl kindly agreed to investigate why root->isInternalPointer() answered no as a separate issue.

It seems like VP arraycopy transformation creates an aiadd/aladd without setting the flag. I'm trying to find where the node is created.

vijaysun-omr · 2018-10-05T18:49:08Z

It seems the title of the PR may have been changed based on @liqunl earlier comment but the PR description and the commit message are still using the words that she requested a change to.

Can you please change the commit message and be more descriptive in terms of why this change was needed ? i.e. under what situation were we using a collected reference register for an aiadd/aladd ?

vijaysun-omr · 2018-10-05T18:53:25Z

@genie-omr build xlinux win

liqunl · 2018-10-05T18:59:26Z

It seems the title of the PR may have been changed based on @liqunl earlier comment but the PR description and the commit message are still using the words that she requested a change to.

I have no problem with the wording now, but I agree that we need a more descriptive commit message.

0dvictor · 2018-10-05T19:10:08Z

Commit messages have been updated. Sanity tests may need be restarted.

liqunl · 2018-10-09T13:00:07Z

Created #3040 to set internal pointer node flags in VP.

andrewcraik · 2018-10-12T13:54:27Z

@vijaysun-omr and @0dvictor I'm not sure if this is the fix we want in general. #3046 was just merged to fix the missing flag. In general we seem to have a principle of offsets to a collected reference result in a collected reference unless the result is explicitly marked as an internal pointer not to be reported to the GC. removing the functionality to allow collected reference to be generated from collected references via explicit offsets not marked internalPointer because Java doesn't use the feature doesn't seem to be quite the right solution. Now there may be another good reason for removing it, but VP failing to set a flag that is required by current convention doesn't seem like a problem the codegen should be trying to fix as alluded to earlier.

0dvictor · 2018-10-12T21:00:32Z

I don't this this PR is needed to fix #3040 either. This is a generic feature fix, while #3046 is the fix for root cause of #3040.

0dvictor · 2018-10-12T21:00:43Z

@andrewcraik raised a good point. I think our different points of views came from different understanding of the semantics of TR::aiadd/TR:aladd. I suggest to wait for #2569 before making decision whether this changeset should be merged.

Marking this PR as WIP for now.

Leonardo2718 · 2018-12-19T16:25:53Z

@0dvictor any updates on this?

0dvictor · 2018-12-19T19:09:20Z

Not yet. I don't think we can progress this further until the semantics of TR::iadd/TR::ladd is clear.

The result register of TR::aiadd/TR::aladd was incorrectly marked as a collected reference, when the aiadd/aladd node is not internal pointer. As a result, an invalid GC map entry may be introduced and causes GC assertions. The result of TR::aiadd/TR::aladd can never be a collected reference, fixing it. Signed-off-by: Victor Ding <[email protected]>

fjeremic added the comp:compiler label Oct 5, 2018

fjeremic requested a review from 0xdaryl October 5, 2018 15:26

vijaysun-omr changed the title ~~Fix for incorrected marked collected reference X86~~ Fix for incorrectly marked collected reference X86 Oct 5, 2018

vijaysun-omr changed the title ~~Fix for incorrectly marked collected reference X86~~ Fix for incorrectly marked collected reference on X86 Oct 5, 2018

0dvictor force-pushed the arraycopy branch from 4a7b7c6 to 80eeb2b Compare October 5, 2018 18:42

vijaysun-omr reviewed Oct 5, 2018

View reviewed changes

0dvictor force-pushed the arraycopy branch from 80eeb2b to efaeee4 Compare October 5, 2018 19:09

0dvictor force-pushed the arraycopy branch from efaeee4 to 44dfc97 Compare October 5, 2018 23:23

pshipton mentioned this pull request Oct 10, 2018

Crashes or assertions in the GC on x86 eclipse-openj9/openj9#3217

Closed

0dvictor changed the title ~~Fix for incorrectly marked collected reference on X86~~ WIP: Fix for incorrectly marked collected reference on X86 Oct 12, 2018

vijaysun-omr self-assigned this Oct 25, 2018

0dvictor force-pushed the arraycopy branch from 44dfc97 to afad5cc Compare December 3, 2018 23:01

0dvictor requested a review from mstoodle as a code owner December 3, 2018 23:01

0dvictor force-pushed the arraycopy branch from afad5cc to 57ed5c2 Compare January 14, 2019 22:59

0dvictor closed this Jan 31, 2019

0dvictor deleted the arraycopy branch January 31, 2019 21:32

hzongaro mentioned this pull request Jul 20, 2023

jdk20 OpenJDK jdk/incubator/vector/Long128VectorTests.java crash vmState=0x0002000f eclipse-openj9/openj9#16979

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Fix for incorrectly marked collected reference on X86 #3035

WIP: Fix for incorrectly marked collected reference on X86 #3035

0dvictor commented Oct 5, 2018 •

edited

Loading

liqunl commented Oct 5, 2018

0dvictor commented Oct 5, 2018 •

edited

Loading

liqunl commented Oct 5, 2018

andrewcraik commented Oct 5, 2018

vijaysun-omr Oct 5, 2018

0dvictor Oct 5, 2018

vijaysun-omr Oct 5, 2018 •

edited

Loading

0dvictor Oct 5, 2018

vijaysun-omr Oct 5, 2018

0dvictor Oct 5, 2018

vijaysun-omr Oct 5, 2018

0dvictor Oct 5, 2018

liqunl Oct 5, 2018

vijaysun-omr commented Oct 5, 2018

vijaysun-omr commented Oct 5, 2018

liqunl commented Oct 5, 2018

0dvictor commented Oct 5, 2018

liqunl commented Oct 9, 2018 •

edited

Loading

andrewcraik commented Oct 12, 2018

0dvictor commented Oct 12, 2018

0dvictor commented Oct 12, 2018 •

edited

Loading

Leonardo2718 commented Dec 19, 2018

0dvictor commented Dec 19, 2018

WIP: Fix for incorrectly marked collected reference on X86 #3035

WIP: Fix for incorrectly marked collected reference on X86 #3035

Conversation

0dvictor commented Oct 5, 2018 • edited Loading

liqunl commented Oct 5, 2018

0dvictor commented Oct 5, 2018 • edited Loading

liqunl commented Oct 5, 2018

andrewcraik commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijaysun-omr Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijaysun-omr commented Oct 5, 2018

vijaysun-omr commented Oct 5, 2018

liqunl commented Oct 5, 2018

0dvictor commented Oct 5, 2018

liqunl commented Oct 9, 2018 • edited Loading

andrewcraik commented Oct 12, 2018

0dvictor commented Oct 12, 2018

0dvictor commented Oct 12, 2018 • edited Loading

Leonardo2718 commented Dec 19, 2018

0dvictor commented Dec 19, 2018

0dvictor commented Oct 5, 2018 •

edited

Loading

0dvictor commented Oct 5, 2018 •

edited

Loading

vijaysun-omr Oct 5, 2018 •

edited

Loading

liqunl commented Oct 9, 2018 •

edited

Loading

0dvictor commented Oct 12, 2018 •

edited

Loading